doc,tools: Replace client-side syntax highlighting for API docs with compile-time syntax highlighting #34148
Conversation
nodejs-github-bot
added
doc
tolmasky
force-pushed
the
syntax-highlight-at-compile-time
branch
3 times, most recently
from
8beb944
to
b607359
Compare
tools/doc/package.json
Outdated
| @@ -16,7 +16,8 @@ | |||
| "unified": "^7.0.0", | |||
| "unist-util-find": "^1.0.1", | |||
| "unist-util-select": "^1.5.0", | |||
| "unist-util-visit": "^1.4.0" | |||
| "unist-util-visit": "^1.4.0", | |||
| "highlightjs": "^9.10.0" | |||
There was a problem hiding this comment.
I'm surprised npm didn't insert this in alphabetical order. Are you using an old version of npm possibly?
There was a problem hiding this comment.
I think this is my fault from doing a rebase/merge. I'll put it in the right place and re-push.
There was a problem hiding this comment.
Ah, a rebase, that makes sense. It doesn't much matter. I was just wondering how it happened.
There was a problem hiding this comment.
I went ahead and fixed it (and rebased again) -- that way if someone else adds a package they won't be confused having it move around.
There was a problem hiding this comment.
Probably an oversight, but the highlightjs package is now deprecated, so you'd want the newer highlight.js one instead.
There was a problem hiding this comment.
👍 Good catch! I went ahead and changed it to highlight.js. Thanks!
|
Seems like a good idea to me, but wondering if @DerekNonGeneric has thoughts they'd like to share. |
tolmasky
force-pushed
the
syntax-highlight-at-compile-time
branch
from
b607359
to
f3fdfe4
Compare
|
I've not looked at the other PR referenced since it doesn't seem related, but please clarify if I'm mistaken. What you are doing in this PR is something that I had considered and decided against. It's also not the way syntax highlighting was being performed in the past.
I don't believe this to be true, but there is a way to confirm. Have you compared the size of the highlight.js bundle to the size difference of the HTML documents before and after you perform SSR of the syntax highlighting? If the differences are larger than the bundle, they will generally take longer to download (there is also HTTP request times to take into account).
The point I've made above affects users w/ slow connections, and almost nobody has JavaScript turned off (especially in the target audience of these documents). Doing what is proposed in this PR will bloat the HTML files, which is why syntax highlighting is a good candidate for client-side JavaScript. Unless I'm missing something, I don't believe these changes are beneficial, however, it's possible that don't understand the motivation behind them. I'd also like to add that I don't really hold too strong of an opinion on the matter, so no big deal if others have a preference. |
Perhaps there is some confusion here (very much possibly on my end), but the only point I was making here is that this change doesn't change the syntax highlighter -- it only changes when the highlighting is done. Both use
From my own tests on a MacBook 2.3 GHz, This is at the cost of a difference of 32KB in gzipped size (729KB for inlined, 697KB for original). Which, depending on cache, is further offset by The individual documentation files are even less of a concern. For example, One other benefit to this is that people can now freely include whatever language they want in the markdown examples without having to recompile |
Prior to this commit, API docs would be highlighted after the page loaded using `highlightjs`. This commit still uses `highlightjs`, but runs the generation during the compilation of the documentation, making it so no script needs to load on the client. This results in a faster load, as well as allowing the pages to fully functional even when JavaScript is turned off.
tolmasky
force-pushed
the
syntax-highlight-at-compile-time
branch
from
f3fdfe4
to
1e73b8c
Compare
|
Just wanting to close the loop on this -- there are no other changes you want me to make right? |
addaleax
added
the
author ready
|
Fwiw, I also don’t have a strong preference here, but I’m okay with landing this and keeping the possibility of changing it back open if somebody complains or longer page load times do turn out to be problematic. |
|
Just following up here again, not sure if what is needed is approval from all the reviewers listed on the right or if anything else is required from me. |
|
@tolmasky, I've removed my thumbs down in case that might help you get this landed. :) |
|
Yeah, thanks for the ping. I’ve kicked off a new CI run. 👍 |
|
Is passing CI a requirement for this commit to go in? I was under the impression that the main branch was failing/flaky generally (just my impression from here: https://github.com/nodejs/node/commits/master). Should I rebase to get this on top of the latest stuff to see if that helps? Or is CI automatically attempting the build as if it was merged on top of the main branch? |
| @@ -3,6 +3,7 @@ | |||
| <head> | |||
| <meta charset="utf-8"> | |||
| <meta name="viewport" content="width=device-width"> | |||
| <meta name="nodejs.org:node-version" content="__VERSION__"> | |||
There was a problem hiding this comment.
@tolmasky, would you mind explaining what this meta tag will be used for (if anything)?
There was a problem hiding this comment.
The problem it solves is that there is currently no easy way to determine what version of the docs you are viewing without falling back regex hacks on the title or h1 element, which should probably not be relied on as a future-proof method, especially when localizing the docs. The URL itself also isn't reliable since these docs often live at vXX-latest. This meta tag provides a place you can reliably query the version of the node docs, whether as a script on the page (as in the other commit), or a program operating on the page (for example if you are running end-to-end tests with puppeteer), or even when visually comparing a diff.
— #34148 (comment)
I wonder if this meta tag would be useful for @zeke's localization work.
@tolmasky Green or yellow – so yes, this is ready and I’ll go ahead and merge it. (Yellow means known flaky tests failed, which is okay.) I know @DerekNonGeneric just posted another comment, but it doesn’t seem like something that needs to be figured out before merging. (As mentioned above, merging something doesn’t need to mean that the conversation has come to an end.)
That impression is correct, unfortunately 😕
Yes, CI automatically rebases, so nothing to do there – rebasing can be helpful if it resolves merge conflicts or it’s a very outdated PR, but otherwise, it can obscure whether new changes have been added that would need to be reviewed and tested separately or not – so no rebasing is definitely okay here. |
|
Landed in fa2e460 |
Prior to this commit, API docs would be highlighted after the page loaded using `highlightjs`. This commit still uses `highlightjs`, but runs the generation during the compilation of the documentation, making it so no script needs to load on the client. This results in a faster load, as well as allowing the pages to fully functional even when JavaScript is turned off. PR-URL: #34148 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Anna Henningsen <[email protected]>
|
Hi @DerekNonGeneric, I wanted to make sure I answered your question: This is a hold-over from the previous commit, but I kept it in because it is generally useful for working in this part of the code base, both for debugging and also for consumers of the page (for example in the companion scripts I wrote to test my changes). The problem it solves is that there is currently no easy way to determine what version of the docs you are viewing without falling back regex hacks on the title or h1 element, which should probably not be relied on as a future-proof method, especially when localizing the docs. The URL itself also isn't reliable since these docs often live at vXX-latest. This meta tag provides a place you can reliably query the version of the node docs, whether as a script on the page (as in the other commit), or a program operating on the page (for example if you are running end-to-end tests with puppeteer), or even when visually comparing a diff. |
Makes sense, thanks! |
These instructions are no longer relevant due to nodejs/node#34148.
Prior to this commit, API docs would be highlighted after the page loaded using `highlightjs`. This commit still uses `highlightjs`, but runs the generation during the compilation of the documentation, making it so no script needs to load on the client. This results in a faster load, as well as allowing the pages to fully functional even when JavaScript is turned off. PR-URL: #34148 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Anna Henningsen <[email protected]>
#131) These instructions are no longer relevant due to nodejs/node#34148.
Prior to this commit, API docs would be highlighted after the page loaded using `highlightjs`. This commit still uses `highlightjs`, but runs the generation during the compilation of the documentation, making it so no script needs to load on the client. This results in a faster load, as well as allowing the pages to fully functional even when JavaScript is turned off. PR-URL: #34148 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Anna Henningsen <[email protected]>
Prior to this commit, API docs would be highlighted after the page loaded using `highlightjs`. This commit still uses `highlightjs`, but runs the generation during the compilation of the documentation, making it so no script needs to load on the client. This results in a faster load, as well as allowing the pages to fully functional even when JavaScript is turned off. PR-URL: #34148 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Anna Henningsen <[email protected]>
Prior to this commit, API docs would be highlighted after the page loaded using `highlightjs`. This commit still uses `highlightjs`, but runs the generation during the compilation of the documentation, making it so no script needs to load on the client. This results in a faster load, as well as allowing the pages to fully functional even when JavaScript is turned off. PR-URL: #34148 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Anna Henningsen <[email protected]>
…s (#131) These instructions are no longer relevant due to nodejs/node#34148.
…s (#131) These instructions are no longer relevant due to nodejs/node#34148.
This PR removes the need to do syntax highlighting at runtime whenever a user loads the API docs by instead doing it during the generation of the documentation. I pulled these changes out of the larger RunKit docs changes since these have been ready to go for a while and I think we could benefit from them now (you can read more here). Originally this change made the RunKit changes significantly simpler to implement, but it has a number of benefits on its own:
To expand a bit more and bring some of the information from the other PR into here:
a. We still use
highlightjsto produce the markup.b. We use the same theme.
c. Syntax highlighting is triggered in the same way from markdown.
highlightjscode that is loaded on the client and run after load, we instead includehighlightjsin the build tools and run it at documentation generation time. For pages like/docs/api/all.html, where the clienthighlightjshas to highlight 27,325 code examples, I think this makes a dramatic difference in the experience (not to mention the electricity we'll be saving spread across all the computers that load these pages).I'm happy to get this back-ported to earlier versions too if someone can direct me to the right way to do that.